lib: fixed unreachable if statements in zlib #24190
Conversation
addaleax
added
the
code-and-learn
| @@ -350,8 +350,7 @@ Object.defineProperty(Zlib.prototype, 'bytesRead', { | |||
| // `params()` function should not happen while a write is currently in progress | |||
| // on the threadpool. | |||
| function paramsAfterFlushCallback(level, strategy, callback) { | |||
| if (!this._handle) | |||
There was a problem hiding this comment.
I think this was an optimization to avoid an unnecessary function call in the common case.
There was a problem hiding this comment.
That should not be a significant overhead. Such a micro optimization should not have any impact on a real application.
lib/zlib.js
Outdated
| } else if (have < 0) { | ||
| assert(false, 'have should not go down'); | ||
| } else { | ||
| assert(have >= 0, 'have should not go down'); |
There was a problem hiding this comment.
| assert(have >= 0, 'have should not go down'); | |
| assert(have === 0, 'have should not go down'); |
There was a problem hiding this comment.
It is not obvious from looking at the code why that check is sufficient. Do you have a small description why? :)
lib/zlib.js
Outdated
| } else if (have < 0) { | ||
| assert(false, 'have should not go down'); | ||
| } else { | ||
| assert(have >= 0, 'have should not go down'); |
There was a problem hiding this comment.
| assert(have >= 0, 'have should not go down'); | |
| assert(have === 0, 'have should not go down'); |
Trott
added
the
author ready
|
you could also change these to CHECK/DCHECK |
|
Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/18625/ |
|
Resume Build: https://ci.nodejs.org/job/node-test-pull-request/18634/ |
|
Took a problematic Windows host offline. Resume Build one more time: https://ci.nodejs.org/job/node-test-pull-request/18635/ |
|
Landed in 4e6d28a Thanks for the contribution! 🎉 (If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.) |
PR-URL: nodejs#24190 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #24190 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #24190 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Refactored assertions in lib/zlib to be reachable
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes